[Refactor]: private model store #7570
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
snewcomer
added
🎯 canary
|
Asset Size Report for afd5d80 IE11 Builds ✅ EmberData shrank by -1007.0 B (-49.0 B compressed)If any packages had changed sizes they would be listed here. Changeset Full Asset Analysis (IE11)Modern Builds ☑️ EmberData shrank by -611.0 B but the compressed size increased slighty (+4.0 B compressed)If any packages had changed sizes they would be listed here. Changeset Full Asset Analysis (Modern)Modern Builds (No Rollup) ✅ EmberData shrank by -1.06 KB (-46.0 B compressed)If any packages had changed sizes they would be listed here. Changeset Full Asset Analysis (Modern) |
|
Performance Report for afd5d80 Scenario - materialization: ☑️ Performance is stable
Scenario - unload: ☑️ Performance is stable
Scenario - destroy: ☑️ Performance is stable
Scenario - add-children: ☑️ Performance is stable
Scenario - unused-relationships: ☑️ Performance is stable
|
snewcomer
force-pushed
the
sn/private-store
branch
from
afca2f6
to
cc37177
Compare
runspired
requested changes
Jun 1, 2021
There was a problem hiding this comment.
Overall this looks good, and I think we should land just-this and separate out the private stuff in a new PR after this lands. Both because this is still an incremental win and because this will help move forward some additional typescript cleanup (setting allowJs: true)
I'm happy to pair with you to go over the typescript stuff, I think it will take some tinkering to come up with the right answers. We may not get it ideal on this pass but we can always iterate on it later.
| identifier: StableRecordIdentifier, | ||
| createRecordArgs: { [key: string]: unknown }, // args passed in to store.createRecord() and processed by recordData to be set on creation | ||
| recordDataFor: (identifier: RecordIdentifier) => RecordDataRecordWrapper, | ||
| createRecordArgs: { [key: string]: any }, |
There was a problem hiding this comment.
this should be unknown still, does it cause type issues if it stays that way?
| createRecordArgs: { [key: string]: unknown }, // args passed in to store.createRecord() and processed by recordData to be set on creation | ||
| recordDataFor: (identifier: RecordIdentifier) => RecordDataRecordWrapper, | ||
| createRecordArgs: { [key: string]: any }, | ||
| recordDataFor: (identifier: StableRecordIdentifier) => RecordDataRecordWrapper, |
There was a problem hiding this comment.
I think RecordIdentifier is the correct API here since a user implementation may not have access to the stable one (they honestly should, but our policy is that APIs that come from user-world don't enforce the object being the stable object, we convert it to the stable-object immediately ourselves instead if it is not already).
| notificationManager: NotificationManager | ||
| ): RecordInstance; | ||
| ): DSModel { |
There was a problem hiding this comment.
let's make this DSModel | RecordInstance. If RecordInstance is causing type issues let's pair and figure out what to put here. It's possible it should be DSModel | unknown or similar since a user can do more or less anything. A generic might be the thing to do here.
|
|
||
| abstract teardownRecord(record: RecordInstance): void; | ||
| teardownRecord(record: DSModel) { | ||
| record.destroy(); |
There was a problem hiding this comment.
let's wrap the destroy call in HAS_MODEL_PACKAGE and a check to ensure the record has a destroy method / or is one of our records.
Similarly the typing of record here needs to be handled similarly to the return type of instantiateRecord. Making sure that return types of instantiateRecord satisfy the typing of teardownRecord is another reason we may want to look at using generics here if RecordInstance is not working well enough.
| } else { | ||
| return this.getSchemaDefinitionService().attributesDefinitionFor(modelName); | ||
| if (identifier) { |
There was a problem hiding this comment.
we can collapse this with the above by making the first if be if (CUSTOM_MODEL_CLASS || !HAS_MODEL_PACKAGE)
| } else { | ||
| return this.getSchemaDefinitionService().relationshipsDefinitionFor(modelName); | ||
| if (identifier) { |
There was a problem hiding this comment.
we can similarly collapse this with the above branching
| throw 'schema service is only available when custom model class feature flag is on'; | ||
| } | ||
| } else { | ||
| if (CUSTOM_MODEL_CLASS) { |
There was a problem hiding this comment.
we can collapse this branching with the HAS_MODEL_PACKAGE branch by only instantiating the ds-model schema definition service in the case that the model package is present.
| let relationshipsByName = get(modelClass, 'relationshipsByName'); | ||
| return relationshipsByName.get(key); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
similar trick for collapsing: if (CUSTOM_MODEL_CLASS || !HAS_MODEL_PACKAGE)
| @@ -76,12 +75,12 @@ if (HAS_MODEL_PACKAGE) { | |||
|
|
|||
| // TODO this should be integrated with the code removal so we can use it together with the if condition | |||
| // and not alongside it | |||
| function isNotCustomModelClass(store: CoreStore | Store): store is Store { | |||
| function isNotCustomModelClass(store: Store): store is Store { | |||
There was a problem hiding this comment.
after this lands we should take a closer look at how this type guard is used. (1) we likely shouldn't need the narrowing behavior anymore and (2) The use-case for that narrowing implies somewhere we will want the ability to specify something like Store<Model> as the more properly typed way of enhancing the instantiateRecord/teardownRecord type for these places in the code.
igorT
suggested changes
Jun 2, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP
HAS_MODEL_PACKAGEto remove code if the end user does not have@ember-data/modelinstalled.- [ ] Move private APIs to_privateStoreas delegate for internal code.